[improvement](catalog) narrow MaterializedIndex addTablet sync scope#64478
Open
yx-keith wants to merge 1 commit into
Open
[improvement](catalog) narrow MaterializedIndex addTablet sync scope#64478yx-keith wants to merge 1 commit into
yx-keith wants to merge 1 commit into
Conversation
Move the synchronized modifier from the public addTablet / appendTablets methods onto appendTabletsInternal, the only block that actually mutates the per-index list and idToTablets map. The TabletInvertedIndex.addTablet(...) call in the non-restore path of addTablet does not need to share this lock — it has its own internal synchronization and operates on a separate object. Holding the per-index monitor across that call only serialized unrelated writers without guarding any shared state on this index. Readers see the new tablets via the same volatile publish in appendTabletsInternal as before, so the existing TOCTOU window between the per-index snapshot and TabletInvertedIndex (handled by the IllegalStateException catch in TabletStatMgr.updateTabletStat) is unchanged in both width and shape.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28621 ms |
Contributor
TPC-DS: Total hot run time: 168410 ms |
Contributor
Author
|
run feut |
Collaborator
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary opinion: no blocking code issue found in this PR. I did not leave inline comments.
Critical checkpoint conclusions:
- Goal and correctness: the PR narrows MaterializedIndex synchronization to the copy-on-write list/map mutation. The changed code still serializes appendTabletsInternal, so concurrent writers cannot lose updates to tablets or idToTablets.
- Scope: the modification is small and focused on one FE catalog class.
- Concurrency: the shared MaterializedIndex snapshots remain volatile and are mutated under the same object monitor. TabletInvertedIndex continues to use its own StampedLock. I did not find a new lock-order or deadlock risk. The temporary visibility gap between the per-index tablet snapshot and TabletInvertedIndex registration is not newly created for readers by this change, and batch creation paths still register tablets before publishing batches.
- Lifecycle/static init/config/compatibility: no new lifecycle management, config, protocol, storage format, or rolling-upgrade compatibility concern was introduced.
- Parallel paths: non-cloud and cloud bulk creation paths already use appendTablets with inverted-index registration before publish; restore paths intentionally skip inverted-index registration and rebuild it separately, unchanged by this PR.
- Transaction/persistence/data visibility: no transaction, EditLog, visible-version, or storage visibility behavior is changed.
- Tests: existing MaterializedIndexTest covers snapshot immutability and concurrent getTablets mutation safety. I attempted to run ./run-fe-ut.sh --run org.apache.doris.catalog.MaterializedIndexTest locally, but generated-source failed before Maven because thirdparty/installed/bin/protoc is missing in this checkout. GitHub/TeamCity currently shows FE UT passing, while check_coverage_fe is failing and should still be resolved before merge.
- Observability/performance: no additional logging or metrics are needed for this lock-scope-only change; the performance direction is reasonable because the inverted-index write lock is no longer held under the MaterializedIndex monitor.
User focus points: none were provided in .code-review.gnVSRV/review_focus.txt.
This was referenced Jun 15, 2026
zclllyybb
added a commit
that referenced
this pull request
Jun 15, 2026
…mpt (#64536) Problem Summary: Automated code review runs relied on the reviewer to infer which module-specific AGENTS.md files apply to a pull request. Recent Litefuse traces showed that reviews often skipped ancestor guides such as the repository root AGENTS.md, fe/AGENTS.md, and be/test/AGENTS.md even when the changed files made them applicable. This change fetches the PR changed file list, derives every existing AGENTS.md file from the changed file ancestor directories, records the list in the review context, and injects the required guide paths directly into the first Codex review prompt. We tested 5 PRs that did not read the AGENTS.md file during previous pipeline runs, and the new results are as follows: | Origin PR | Test PR | Run | Litefuse Trace | Result | |---|---:|---:|---|---| | #64478 | zclllyybb#36 | `27536249602` | `d76b77ac4e0d52d96f413b154c9f2571` | Read All | | #64458 | zclllyybb#37 | `27536258049` | `8db34388a8379acdcd9e48720f11225f` | Read All | | #64392 | zclllyybb#38 | `27536266784` | `a5e37fba03375a9517f7a67e33f70d39` | Read All | | #64489 | zclllyybb#39 | `27536275680` | `22ace18d70ece96b0ca7fa73123b62da` | Read All | | #64419 | zclllyybb#41 | `27538239601` | `1803c040e4140d61e840e4e1526190e7` | Read All |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Move the synchronized modifier from the public addTablet / appendTablets methods onto appendTabletsInternal, the only block that actually mutates the per-index list and idToTablets map.
The TabletInvertedIndex.addTablet(...) call in the non-restore path of addTablet does not need to share this lock — it has its own internal synchronization and operates on a separate object. Holding the per-index monitor across that call only serialized unrelated writers without guarding any shared state on this index.
Readers see the new tablets via the same volatile publish in appendTabletsInternal as before, so the existing TOCTOU window between the per-index snapshot and TabletInvertedIndex (handled by the IllegalStateException catch in TabletStatMgr.updateTabletStat) is unchanged in both width and shape.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #63298
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)